Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix popover with very narrow text. #1894

Merged
merged 2 commits into from
May 16, 2024

Conversation

kevin1
Copy link
Contributor

@kevin1 kevin1 commented May 15, 2024

#1702

If the page is resized before any footnotes have been opened, the resize action can compute a very small max-width based on zero size footnote contents. This was allowed to happen because isMounted always returns true after the popover element is created but not yet shown.

  • isMounted now checks whether the popover is contained in the DOM.
  • createElementFromHTML now unwraps newly created elements to improve robustness in case parentElement gets used elsewhere.

goblindegook#1702

If the page is resized before any footnotes have been opened, the
`resize` action can compute a very small `max-width` based on zero size
footnote contents. This was allowed to happen because `isMounted` always
returns true after the popover element is created but not yet shown.

- `isMounted` now checks whether the popover is contained in the DOM.
- `createElementFromHTML` now unwraps newly created elements to improve
  robustness in case `parentElement` gets used elsewhere.
@goblindegook
Copy link
Owner

Thanks for this, would you be able to cover the change with a test? (Probably using Cypress since resizing the browser is one of the steps to reproduce.)

@kevin1
Copy link
Contributor Author

kevin1 commented May 16, 2024

Here is the test which fails when the fix is undone:

Click -- popover layout upon resizing window before activation (#1702) (failed)

@goblindegook goblindegook merged commit 5c7633e into goblindegook:main May 16, 2024
@kevin1 kevin1 deleted the fix-narrow-popover branch May 16, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants